-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve pricing options flexibility #2504
Improve pricing options flexibility #2504
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good addition to the API. Please do add some specs.
2329719
to
6626470
Compare
subject { described_class.from_context(view_context) } | ||
|
||
context "if the store has not defined default_currency" do | ||
let(:store) { FactoryBot.create :store, default_currency: nil , cart_tax_country_iso: nil} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space found before comma.
Space missing inside }.
@@ -68,6 +68,29 @@ | |||
end | |||
end | |||
|
|||
context ".from_context" do | |||
let(:view_context) { double(ApplicationController, current_store: store)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space missing inside }.
6626470
to
73a3bcc
Compare
Done @mamhoff, let me know what you think now... Thanks! |
Looks good, but can you look into the failing spec? |
Hm. The spec failure fix makes me wary. Why do the first two commits change the return value of |
Actually, the failing spec was a test that sometimes fails, I saw it red in other PR's. My guess is that a simple rebuild would've made it green. I didn't change |
Any comment @mamhoff ? I can rollback the view change if you feel it is not that safe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine. Go for it.
3b365ab
to
9ebda2f
Compare
9ebda2f
to
1606b79
Compare
subject { described_class.from_context(view_context) } | ||
|
||
context "if the store has not defined default_currency" do | ||
let(:store) { FactoryBot.create :store, default_currency: nil , cart_tax_country_iso: nil } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layout/SpaceBeforeComma: Space found before comma.
1606b79
to
051e470
Compare
051e470
to
f34064b
Compare
@jtapia Hey I think this is a great change, can you please rebase against master? |
@softr8 thanks for this PR 👏 |
f34064b
to
dc40bc9
Compare
Rebased! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @softr8!
Thanks @softr8 !! |
The last piece, I think, to make pricing options totally flexible is to allow how it's built from a view context, allowing developers to decide how to find the prices based, example: users role
It also adjusts the API to use
object.price_for(current_pricing_options)
I can add tests if you think this is a good idea.